-
-
Notifications
You must be signed in to change notification settings - Fork 223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow users update their account email address with proper validation. #4520
feat: allow users update their account email address with proper validation. #4520
Conversation
@apsinghdev is attempting to deploy a commit to the polar-sh Team on Vercel. A member of the Team first needs to authorize it. |
@frankie567 I've raised this draft PR that contains backend code. please have a look and let me know if it make sense. (There are further improvements like tags that I'll make this week) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great work @apsinghdev 👍 Let's change a few things to make it really tailored for email verification :)
Be sure also to run linters:
uv run task lint && uv run task lint_types
@frankie567 Thanks for the reviews. I'll fix these very soon. |
6c85b89
to
8a54988
Compare
@frankie567 I've made the updates to apply reviews and implemented client-side logic and UI. Please have a look whenever you are free and let me know your thoughts. Demo: Screen.Recording.2024-12-01.at.10.25.19.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're getting there, great work @apsinghdev 👍 Just a few adjustments and we should be able to land this.
7c77a76
to
b001aff
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@frankie567 I have made the changes. Please have a look whenever you get the chance. Demo: Screen.Recording.2024-12-08.at.10.40.52.PM.1.mov |
3041eac
to
f59a2d0
Compare
(Already posted on discord, writing here just for the reference) You might have a question about why there is a verify-email route. Actually, I tried to keep the whole update process in the settings itself but it was causing a problem of not updating the email in the first render because of the async nature of react hooks. I had to refresh the page to verify the email. I tried these ways to solve it: Catching the token using urlSeachParams and then triggering a hit to API to verify Creating a route.ts to intercept the request midway and hit api Using server actions. In all of these methods, We had to depend on client-side rendering to provide us the token in the URL to take further actions to verify but by default, the very first render doesn't set the token properly. So I considered creating a server-side page verify-email to avoid dependency on client-side rendering and verify the user. If you have a better idea to implement it, please let me know. I'll be happy to make changes. |
f59a2d0
to
c19fe2a
Compare
Hey @apsinghdev 👋 That's great work, thanks! So, I took the liberty to rebase your PR and make some slight changes to the UI. Basically, it's now nested under the Signin connections, but the logic remains the same (really liked your approach with a Record of components to render depending on the status ;) ) ![]() ![]() ![]() ![]() During my tests, I found one big issue we need to resolve: when requesting the email change, we do not check if the email already exists in the database, which causes an issue when trying to update the email. Could you do that? Basically, at the beginning of Thank you 🙏 |
@frankie567 Thank you for the reviews. I'll make the changes very soon. 🙂 |
c19fe2a
to
492125b
Compare
Hi @frankie567, I've made the necessary changes to check if an email already exists or not. Here is how it looks: Screen.Recording.2024-12-17.at.11.14.32.PM.movPlease take a look when you are free. |
bcd735d
to
412dbbc
Compare
Thank you @apsinghdev 👍 Made a few tweaks + a rebase, it's good to go now 😄 Thank you for your hard work 🙇 |
Thank you so much for your valuable feedbacks. 🙏😊 |
fixes: #4237
This PR implements the feature to allow users to update their email. (Draft)
Screen.Recording.2024-12-01.at.10.25.19.PM.mov